fix: render extension skills for target agent, not just active agent …#3599
fix: render extension skills for target agent, not just active agent …#3599Nimraakram22 wants to merge 3 commits into
Conversation
…ithub#2948) Extension skill rendering was previously scoped only to the active agent (init-options.json's 'ai' field), even when register_enabled_extensions_for_agent was called for a different, non-active agent (e.g. after 'integration install <agent> --skills' or 'integration upgrade <agent>' for a secondary integration). This meant a skills-mode agent that wasn't the active one received command files instead of skills. Changes: - Add is_agent_skills_enabled() in _init_options.py: resolves per-agent skills-mode from .specify/integration.json's integration_settings[agent].parsed_options.skills, falling back to the legacy global init-options ai_skills flag for the active agent. - ExtensionManager._get_skills_dir() now accepts an optional agent_name and resolves the skills directory for that specific agent instead of always using the active agent. - ExtensionManager._register_extension_skills() now accepts and threads through agent_name. - register_enabled_extensions_for_agent() now computes skills_mode_active per the target agent_name (not the active agent), and always attempts skill registration for that agent (matching the prior best-effort error handling), while still skipping duplicate command-file registration when the target is in skills mode. Verified manually: 'specify integration upgrade copilot --force' with a non-active, skills-mode Copilot integration now renders .github/skills/speckit-git-*/SKILL.md files instead of command files, with no change to the active agent's own skills. Test suite: 616/616 extension-related tests pass. Full suite has 44 pre-existing failures unrelated to this change (Windows symlink-privilege and PowerShell path-resolution issues, confirmed present on main via git stash).
There was a problem hiding this comment.
Pull request overview
Makes extension skill rendering aware of the target integration rather than only the active agent.
Changes:
- Resolves per-agent skills mode from integration state.
- Threads target agent through skill-directory resolution and rendering.
- Updates extension re-registration behavior and documentation.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/_init_options.py |
Adds per-agent skills-mode resolution. |
src/specify_cli/extensions/__init__.py |
Targets extension skill generation by agent. |
src/specify_cli/integrations/_helpers.py |
Updates registration behavior documentation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
src/specify_cli/extensions/init.py:980
- The target-agent branch bypasses the repository's safe directory creation.
resolve_agent_skills_dironly constructs a path, and_ensure_usablefollows symlinked parents withmkdir;_register_extension_skillsthen writesSKILL.mdbeneath it. A project with (for example) a symlinked.github/skillscan therefore make registration write outside the project. Route this path through_ensure_safe_shared_directoryasresolve_active_skills_dirdoes (seeshared_infra.py:163-196) before writing.
skills_dir = resolve_agent_skills_dir(self.project_root, agent_name)
src/specify_cli/extensions/init.py:1856
registered_skillsis still a single global list of names, so merging a second agent's identically named skills loses which directories own them. For example, after Claude and non-active Copilot both renderspeckit-git-feature, uninstalling either agent removes its copy and clears the list inunregister_agent_artifacts; the other copy remains but is no longer tracked, so later extension removal leaves it orphaned. Track generated skills per agent (with legacy-list compatibility) before enabling multi-agent skill rendering.
existing_skills = self._valid_name_list(
metadata.get("registered_skills", [])
)
merged_skills = list(
dict.fromkeys(existing_skills + registered_skills)
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Medium
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback and address test & lint errors
…thub#2948) Addresses PR review feedback: install_from_directory() (the 'extension add' / dev-install path) still called _register_extension_skills() with no agent_name, so it remained scoped to only the active agent, unlike register_enabled_extensions_for_agent() which was already fixed to target a specific agent. Changes: - Add ExtensionManager._register_extension_skills_for_installed_agents(), which renders skills for the active agent (legacy single-agent projects) plus every agent listed in .specify/integration.json's installed_integrations, for whichever of those are individually in skills mode. Failures for one agent are warned and do not block the others. - install_from_directory() now calls this helper instead of a single active-agent-only call. - _get_skills_dir()'s agent_name branch now mirrors resolve_active_skills_dir()'s safety checks (_ensure_safe_shared_directory symlink/containment/is-a-directory validation, plus the Kimi native-skills-dir-must-already-exist fallback) instead of resolving the naive per-agent path unchecked — fixes a regression this exposed in the Hermes marker-file detection test and restores exact parity with the active-agent path. Tests: - Added TestNonActiveAgentSkillRegistration with 3 new tests covering register_enabled_extensions_for_agent for a non-active skills-mode agent, isolation from the active agent's own skills, and extension add rendering skills for all installed skills-mode agents. - pytest tests/ -k extension: 619/619 pass (616 previous + 3 new). - Manually verified: 'specify extension add git' with Claude active and Copilot installed in --skills mode now renders SKILL.md files for both agents ('5 agent skill(s) auto-registered'), with no change to Claude's own skills. - git diff --check: clean, no whitespace errors.
|
@mnriem Thanks for the review! Pushed a follow-up commit addressing both points:
Also had to fix a regression this exposed: my agent-specific
|
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
src/specify_cli/extensions/init.py:1569
- Adding this second skill pass does not stop the preceding
register_commands_for_all_agentspass from registering command files for the same skills-mode agent. For example, if.github/agentsalready exists (often because it contains user agents), a persisted Copilot--skillsinstall receives both.agent.mdfiles andSKILL.md, violating the acceptance criterion. Filter per-agent skills-mode integrations out of command registration rather than merely adding skills afterward.
# Auto-register extension commands as agent skills for every
# skills-mode agent detected (active agent plus any other
# installed integrations in skills mode), not just the active
# agent (#2948).
registered_skills = self._register_extension_skills_for_installed_agents(
manifest, dest_dir, link_outputs=link_commands
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
| Extension *skill* rendering is scoped to ``agent_key`` itself (using | ||
| per-agent settings in ``.specify/integration.json`` when available, with | ||
| a fallback to the legacy global init-options for the active agent), so a | ||
| skills-mode agent registered while it is not the active agent (e.g. | ||
| Copilot ``--skills`` registered while non-active) still receives skill | ||
| files here instead of only command files (#2948). |
| for skill_name in agent_skills: | ||
| if skill_name not in seen: | ||
| seen.add(skill_name) | ||
| combined.append(skill_name) |
|
Please address Copilot feedback and fix test & lint errors |
Addresses Copilot review feedback: remove() called _unregister_extension_skills() without a directory, so its fast path removed skill files only from the resolved active-agent directory whenever one resolved, and never scanned the directories of other agents whose skills were rendered via the new multi-agent registration paths added earlier in this PR — leaving those orphaned on the filesystem after removal. _unregister_extension_skills() now always scans every known agent skills directory (previously this broader scan only ran as a fallback when no active-agent directory could be resolved at all), in addition to any explicitly-passed or resolved directory. Each candidate directory's matching skill subdirectories are still verified against the SKILL.md metadata.source field before removal, so this doesn't touch the existing safety guarantees or the registered_skills schema (intentionally kept as a flat, deduped list rather than a per-agent dict, to avoid a breaking registry-schema change for this fix). Tests: - Added test_remove_cleans_up_skills_for_non_active_agent, covering removal after skills were rendered for both an active and a non-active agent. - pytest tests/ -k extension: 620/620 pass (619 previous + 1 new). - git diff --check: clean. Note: a related Copilot suggestion to also have plain 'integration install' immediately register extension commands/skills for the newly-installed agent was evaluated but not applied here, since it directly conflicts with the maintainer-requested, explicitly-tested deferred-registration design from github#2886 (see tests/integrations/test_integration_subcommand.py::TestIntegrationInstall::test_install_defers_extension_commands_until_use). Flagging this for maintainer input rather than silently reversing that prior decision.
|
@mnriem Pushed another commit addressing Copilot's second point (skill cleanup across agents):
On the first point (making plain Since that looks like a deliberate, previously-approved decision, I didn't want to silently reverse it based on an automated review comment. @mnriem should
|
The existing behavior should be retained
|
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
src/specify_cli/integrations/_helpers.py:410
- Plain
integration installstill never invokes this helper (the docstring immediately above says install deliberately has no extension side effects, and the call sites are only use/switch/upgrade). Therefore the issue's reproduction—add the extension, then install non-active Copilot with--skills --force—still leaves Copilot without the extension skill. Wire the install path into this agent-aware registration, or otherwise reconcile the stated install acceptance criterion.
Extension *skill* rendering is scoped to ``agent_key`` itself (using
per-agent settings in ``.specify/integration.json`` when available, with
a fallback to the legacy global init-options for the active agent), so a
skills-mode agent registered while it is not the active agent (e.g.
Copilot ``--skills`` registered while non-active) still receives skill
files here instead of only command files (#2948).
src/specify_cli/extensions/init.py:1574
- This second pass adds skills but does not prevent the preceding
register_commands_for_all_agents()pass from writing command files for a non-active skills-mode agent. That registrar only recognizes skills mode from the activeinit-optionsagent (agents.py:1008-1010), so an installed Copilot with.github/agentspresent receives both.agent.mdandSKILL.md, contrary to the acceptance criterion. The new test masks this by callinginstall_from_directory(..., register_commands=False); the normal extension-add path needs to filter per-agent command registration and assert the.agent.mdfile is absent.
# Auto-register extension commands as agent skills for every
# skills-mode agent detected (active agent plus any other
# installed integrations in skills mode), not just the active
# agent (#2948).
registered_skills = self._register_extension_skills_for_installed_agents(
manifest, dest_dir, link_outputs=link_commands
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
| # Additionally always scan every other known agent skills | ||
| # directory: skills may have been rendered for more than one | ||
| # agent (#2948), so limiting cleanup to only the directory above | ||
| # would orphan skills registered for a non-active agent. | ||
| # Directories already handled above are silently skipped since | ||
| # their matching skill subdirectories no longer exist. | ||
| if True: |
|
Address Copilot feedback and fix test & lint errors. Note that cleanup should never touch any files it does not own. So if a specific integration is uninstalled it should only cleanup those files |
Fixes #2948.
Problem
Extension skill rendering was scoped to the active agent (init-options.json's
aifield) even when registering extensions for a different, non-active agent — e.g. afterspecify integration install copilot --integration-options "--skills"while Claude remained active, orspecify integration upgrade copilot. The target agent received command files instead of SKILL.md files.Root cause
ExtensionManager._get_skills_dir()and_register_extension_skills()always resolved the skills directory/settings from the active agent ininit-options.json, ignoring theagent_nameargument passed toregister_enabled_extensions_for_agent().Fix
is_agent_skills_enabled()to resolve per-agent skills-mode from.specify/integration.json'sintegration_settings[agent].parsed_options.skills, falling back to the legacy globalai_skillsflag for the active agent.agent_namethrough_get_skills_dir()and_register_extension_skills().register_enabled_extensions_for_agent()now computesskills_mode_activeper the target agent, and always attempts skill registration for that agent (matching prior best-effort error handling).Testing
specify integration upgrade copilot --forcewith a non-active, skills-mode Copilot integration now renders.github/skills/speckit-git-*/SKILL.mdfiles instead of command files, with no regression to the active agent's own skills.pytest tests/ -k extension: 616/616 pass.mainwithout this change).